Benchmark Results (Julia v1.10)

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

I fixed all the issues JET.jl v0.11 found apart from three:

• local variable `accumulated_data` may be undefined: accumulated_data::TimerOutputs.TimeData: This probably comes from TrixiBase.jl. I guess it's https://github.com/trixi-framework/TrixiBase.jl/blob/b65bea70512e393bc82bd64a09765ad0c74bc586/src/trixi_timeit.jl#L66. I'll see if I can fix it there. Edit: Should be fixed by Fix issues found by JET.jl trixi-framework/TrixiBase.jl#57.
• local variable `psi` may be undefined: psi::Any: That's a bit tricky. I see why JET.jl complains about this part. But it's not so easy to fix it nicely because we cannot merge the if statements easily and it's also not so nice to always unpack psi again in each if statement. Any idea, @ranocha?
• no matching method found `is_key_supported(::Symbol)`: (RecipesBase).is_key_supported::typeof(RecipesBase.is_key_supported)(:plot_initial): This comes from RecipesBase.jl and is probably why we needed the workaround before (?). I'll need to check if we can ignore these somehow with JET.jl v0.11 (ignored_modules = (RecipesBase,) doesn't help). Edit: I fixed that by importing Plots.jl before testing, which makes is_key_supported available.

That's a bit tricky. I see why JET.jl complains about this part. But it's not so easy to fix it nicely because we cannot merge the if statements easily and it's also not so nice to always unpack psi again in each if statement.

You can report this as a sub-optimal result of JET.jl. From looking at the code, I do not see an obvious way how psi could not be defined there.

To fix this right now, the only option I see is to unpack psi every time from the cache.

That's a bit tricky. I see why JET.jl complains about this part. But it's not so easy to fix it nicely because we cannot merge the if statements easily and it's also not so nice to always unpack psi again in each if statement.

You can report this as a sub-optimal result of JET.jl. From looking at the code, I do not see an obvious way how psi could not be defined there.

Yes, JET.jl is not smart enough to realize that the first first if equations.bathymetry_type isa BathymetryVariable condition will always be true and therefore psi will be defined when the other if equations.bathymetry_type isa BathymetryVariable conditions are true. That's also basically the same issue with the first problem from @trixi_timeit in TrixiBase.jl.
I now always unpack psi from the cache.

Did you file an issue in their repo? They should know about false positives like this.

Did you file an issue in their repo? They should know about false positives like this.

No because this is the same as aviatesk/JET.jl#749. So the compiler cannot infer that in the second branch the variable is defined and we need to resort to workarounds. I am not a big fan of adding assertions in the branches as was also suggested in the issue above. So I think the only way to make JET.jl pass is using the workarounds we already found (unpacking psi in every branch and code duplication in TrixiBase.jl).